cargo-llvm-cov: fix tests, mark broken as needed by CobaltCause · Pull Request #197478 · NixOS/nixpkgs

CobaltCause · 2022-10-24T03:29:27Z

Description of changes

~~This packages cargo-llvm-cov, a tool for generating code coverage reports using built-in features of recent Rust toolchains.~~

~~Supersedes #197453~~
~~Alternative to #185183~~

Notably, this PR has all the tests running and passing, and does not wrap the binary, allowing users to choose their own toolchain versions.

Things done

winterqt

So the change I want to try here is using fetchurl and postFetch to not only get the lockfile from crates.io, but collapse it into a single FOD to take advantage of caching.

Essentially, use fetchurl on https://crates.io/api/v1/crates/${pname}/${version}/download, with downloadToTemp set to true, and do something like tar xzf $downloadedFile ${pname}-${version}/Cargo.lock; mv */Cargo.lock $out in postFetch:

fetchurl {
  name = "Cargo.lock";
  url = "https://crates.io/api/v1/crates/${pname}/${version}/download";
  downloadToTemp = true;
  postFetch = ''
    tar xzf $downloadedFile ${pname}-${version}/Cargo.lock
    mv */Cargo.lock $out
  '';
}

Then, copy this into the source in postPatch and cargoUpdateHook, which should then allow us to run tests while not having to include the lockfile.

winterqt · 2022-10-24T03:53:24Z

Though, now that I say that, I wonder if the compressed source tarball is smaller than the uncompressed Cargo.lock, lol. I'd guess that it's bigger, but who knows with the sourcery that is compression.

CobaltCause · 2022-10-24T03:59:18Z

Cursed: compress the checked-in Cargo.lock for maximum smallness.

I'll try implementing your suggestion. Is the concern mainly the size of the nixpkgs repository or the size of the stuff required to build the derivation?

pkgs/development/tools/rust/cargo-llvm-cov/default.nix

winterqt · 2022-10-24T04:06:41Z

I'd say checking in large autogenerated files is worse than doing this, but at the same time... this is a pretty small lockfile. Hrm.

Unrelated to this, but we currently disable the profiler runtime on non-Linux platforms. Given this, can you set meta.broken = !(stdenv.isLinux && stdenv.targetPlatform.isRedox)? (Maybe there's a better clause to use, not sure if we care about Redox here.)

(If you have Darwin hardware, feel free to try to enable it and test it -- if not, I'll do it sometime this week, I just don't want to have this package have to wait for a staging cycle.)

CobaltCause · 2022-10-24T04:16:09Z

I'd say checking in large autogenerated files is worse than doing this, but at the same time... this is a pretty small lockfile. Hrm.

Well, good news: your trick worked perfectly. And yeah I agree that checking in autogenerated stuff is suboptimal.

Unrelated to this, but we currently disable the profiler runtime on non-Linux platforms. Given this, can you set meta.broken = !(stdenv.isLinux && stdenv.targetPlatform.isRedox)? (Maybe there's a better clause to use, not sure if we care about Redox here.)

Sure. EDIT: One thing I'm curious about is whether this makes sense/still applies when getting your rust toolchain from somewhere other than nixpkgs, e.g. fenix.

(If you have Darwin hardware, feel free to try to enable it and test it -- if not, I'll do it sometime this week, I just don't want to have this package have to wait for a staging cycle.)

I don't, but I have a friends with x86_64-darwin and aarch64-darwin that I may be able to rope into this.

pkgs/development/tools/rust/cargo-llvm-cov/default.nix

winterqt · 2022-10-24T13:12:05Z

@Mic92 @zowoq Thoughts on 09b7abe1a29c4c47982984d8ea092b9c50031048, just to use cargoUpdateHook to copy a Cargo.lock into the derivation?

There's another option we can take here, and that's adding a way to specify a postPatch that gets passed to fetchCargoTarball when using buildRustPackage, but I think this is probably the least impactful (since only one other package even uses cargoUpdateHook) since we already have it (cargoUpdateHook).

CobaltCause · 2022-11-09T00:12:58Z

Looks like someone else beat us to it: #192813

Do we want to close this, or should I rebase on top of their changes so we can have working tests and a proper meta.broken?

winterqt · 2022-11-09T00:14:35Z

Let's do that, and adjust the commit message(s?) appropriately, please.

Sorry about forgetting this.

CobaltCause · 2022-11-09T00:19:26Z

No problem! I can do that, yes. Do we want to leave the changes to fetchCargoTarball or get rid of it since it's not needed anymore?

winterqt · 2022-11-10T04:32:15Z

Do we want to leave the changes to fetchCargoTarball or get rid of it since it's not needed anymore?

Probably best to move it to a separate PR, so this isn't blocked on any (potential) issues/discussions.

CobaltCause · 2022-11-10T20:12:56Z

Okay, rebase is done and fetchCargoTarball changes are gone. The diff is pretty big because I just kind of blindly rebased, should I break it into smaller commits?

figsoda · 2023-08-12T01:42:24Z

oops, merge conflict again

pkgs/development/tools/rust/cargo-llvm-cov/default.nix

matthiasbeyer

I think we can merge this now, can't we?

CobaltCause · 2023-08-24T06:16:41Z

I mean, I think so, but I'm obviously biased. And it would be nice to get an approval from @winterqt since they've invested a lot of time helping with this and reviewing it though.

Waited for three weeks but no reply.

matthiasbeyer · 2023-09-11T07:52:44Z

I assume that they won't reply, so I'll merge after some nixpkgs-review call.

matthiasbeyer · 2023-09-11T07:53:20Z

Result of nixpkgs-review pr 197478 run on x86_64-linux 1

1 package built:

cargo-llvm-cov

CobaltCause mentioned this pull request Oct 24, 2022

cargo-llvm-cov: init at 0.5.0 #197453

Closed

13 tasks

winterqt requested changes Oct 24, 2022

View reviewed changes

figsoda reviewed Oct 24, 2022

View reviewed changes

pkgs/development/tools/rust/cargo-llvm-cov/default.nix Outdated Show resolved Hide resolved

CobaltCause force-pushed the add-cargo-llvm-cov branch from 1415724 to a606f1e Compare October 24, 2022 04:20

winterqt requested changes Oct 24, 2022

View reviewed changes

pkgs/development/tools/rust/cargo-llvm-cov/default.nix Outdated Show resolved Hide resolved

CobaltCause force-pushed the add-cargo-llvm-cov branch from a606f1e to 92538d7 Compare October 24, 2022 05:54

github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Oct 24, 2022

winterqt mentioned this pull request Oct 24, 2022

cargo-llvm-cov: init at 0.4.13 #185183

Closed

13 tasks

CobaltCause force-pushed the add-cargo-llvm-cov branch from 92538d7 to 6ff7457 Compare October 24, 2022 17:33

CobaltCause force-pushed the add-cargo-llvm-cov branch from 6ff7457 to ea86614 Compare November 10, 2022 17:02

github-actions bot removed the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Nov 10, 2022

ofborg bot requested a review from wucke13 November 10, 2022 17:13

CobaltCause requested review from winterqt and removed request for wucke13 November 10, 2022 20:14

CobaltCause force-pushed the add-cargo-llvm-cov branch from af3a9a5 to cc0cede Compare August 11, 2023 21:52

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 12, 2023

CobaltCause force-pushed the add-cargo-llvm-cov branch from cc0cede to 8e11811 Compare August 12, 2023 02:52

CobaltCause requested a review from figsoda August 12, 2023 02:53

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 12, 2023

ofborg bot requested a review from matthiasbeyer August 12, 2023 03:27

winterqt previously requested changes Aug 12, 2023

View reviewed changes

pkgs/development/tools/rust/cargo-llvm-cov/default.nix Outdated Show resolved Hide resolved

CobaltCause force-pushed the add-cargo-llvm-cov branch from 8e11811 to 1f9d679 Compare August 16, 2023 21:31

CobaltCause requested a review from winterqt August 17, 2023 02:40

matthiasbeyer approved these changes Aug 17, 2023

View reviewed changes

CobaltCause force-pushed the add-cargo-llvm-cov branch from 1f9d679 to a282bd4 Compare August 23, 2023 19:27

ofborg bot requested a review from matthiasbeyer August 23, 2023 19:59

matthiasbeyer approved these changes Aug 24, 2023

View reviewed changes

wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Sep 7, 2023

CobaltCause added 4 commits September 10, 2023 15:58

cargo-llvm-cov: add myself as a maintainer

0bf98c4

cargo-llvm-cov: mark as broken on some platforms

8ad7950

cargo-llvm-cov: refactor to fix tests

eae65c5

cargo-llvm-cov: document test failure modes

cefb386

CobaltCause force-pushed the add-cargo-llvm-cov branch from a282bd4 to cefb386 Compare September 10, 2023 23:03

ofborg bot requested a review from matthiasbeyer September 10, 2023 23:22

matthiasbeyer approved these changes Sep 11, 2023

View reviewed changes

matthiasbeyer merged commit ac1367d into NixOS:master Sep 11, 2023

CobaltCause deleted the add-cargo-llvm-cov branch September 11, 2023 14:28

benalleng mentioned this pull request Dec 4, 2025

Build failure: cargo-llvm-cov #467882

Closed

3 tasks

Uh oh!

Conversation

CobaltCause commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

winterqt left a comment

Choose a reason for hiding this comment

Uh oh!

winterqt commented Oct 24, 2022

Uh oh!

CobaltCause commented Oct 24, 2022

Uh oh!

Uh oh!

winterqt commented Oct 24, 2022

Uh oh!

CobaltCause commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

winterqt commented Oct 24, 2022

Uh oh!

CobaltCause commented Nov 9, 2022

Uh oh!

winterqt commented Nov 9, 2022

Uh oh!

CobaltCause commented Nov 9, 2022

Uh oh!

winterqt commented Nov 10, 2022

Uh oh!

CobaltCause commented Nov 10, 2022

Uh oh!

figsoda commented Aug 12, 2023

Uh oh!

Uh oh!

matthiasbeyer left a comment

Choose a reason for hiding this comment

Uh oh!

CobaltCause commented Aug 24, 2023

Uh oh!

matthiasbeyer commented Sep 11, 2023

Uh oh!

matthiasbeyer commented Sep 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CobaltCause commented Oct 24, 2022 •

edited

Loading

CobaltCause commented Oct 24, 2022 •

edited

Loading